[Enhancement] Make filter section collapsible to improve popup layout#387
[Enhancement] Make filter section collapsible to improve popup layout#387gurusatsangi wants to merge 11 commits intofossasia:mainfrom
Conversation
Reviewer's GuideWraps the existing popup filter controls in a new "Advanced Filters" collapsible section, wired up with a toggle button and icon rotation, without changing any underlying filter logic or API behavior. Sequence diagram for Advanced Filters toggle behavior in the popupsequenceDiagram
actor User
participant PopupUI
participant PopupScript
User->>PopupUI: Click toggleAdvancedFilters button
PopupUI->>PopupScript: click event on toggleAdvancedFilters
PopupScript->>PopupUI: Toggle hidden class on advancedFiltersContent
PopupScript->>PopupUI: Toggle rotate-180 class on advancedFiltersArrow
PopupUI->>User: Update layout and arrow orientation
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
github-actions
bot
added
javascript
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider adding basic accessibility attributes to the toggle (e.g.,
aria-expandedon the button andaria-controls/aria-hiddenon the content container) and keep them in sync when opening/closing the advanced filters section. - The toggle label text is duplicated between the HTML (
Advanced Filters ▼) and the JavaScript ("Advanced Filters ▼"/"Advanced Filters ▲"); it would be more maintainable to derive the label from a single source or use CSS to render the arrow indicator. - The global
.hidden { display: none; }utility might conflict with other styles; if this project already has a hide/show helper or scoped CSS, consider reusing or namespacing this class instead of adding a generic one.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider adding basic accessibility attributes to the toggle (e.g., `aria-expanded` on the button and `aria-controls`/`aria-hidden` on the content container) and keep them in sync when opening/closing the advanced filters section.
- The toggle label text is duplicated between the HTML (`Advanced Filters ▼`) and the JavaScript (`"Advanced Filters ▼"` / `"Advanced Filters ▲"`); it would be more maintainable to derive the label from a single source or use CSS to render the arrow indicator.
- The global `.hidden { display: none; }` utility might conflict with other styles; if this project already has a hide/show helper or scoped CSS, consider reusing or namespacing this class instead of adding a generic one.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
This PR is ready for review. It introduces a collapsible Advanced Filters section to improve the popup layout and reduce vertical clutter. All existing functionality has been tested locally (report generation, dark mode, toggles, etc.). Kindly review when convenient. |
vedansh-5
left a comment
vedansh-5
left a comment
There was a problem hiding this comment.
Thanks @gurusatsangi for your contributions!
src/index.css
Outdated
| .advanced-filters-btn { | ||
| width: 100%; | ||
| padding: 8px; | ||
| margin-top: 10px; | ||
| background-color: #f3f4f6; | ||
| border: 1px solid #d1d5db; | ||
| border-radius: 6px; | ||
| cursor: pointer; | ||
| font-weight: 500; | ||
| } | ||
|
|
||
| .advanced-filters-content { | ||
| margin-top: 8px; | ||
| padding: 8px; | ||
| border: 1px solid #e5e7eb; | ||
| border-radius: 6px; | ||
| } | ||
|
|
||
| .hidden { | ||
| display: none; | ||
| } |
There was a problem hiding this comment.
Please use the same styling as existing labels and replace the dropdown triangle with an arrow
Like the one in platform selector dropdown
There was a problem hiding this comment.
ok i will working on it and do it by tommorow
|
@vedansh-5 Updated the Advanced Filters styling to fully match the platform selector dropdown (same classes, spacing, and chevron arrow). Removed custom CSS and aligned with existing design. Please review again. |
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #385 by wrapping the existing filter checkboxes (show PRs labels, only issues, only PRs, only reviewed PRs, show commits) inside a collapsible "Advanced Filters" section with a toggle button and an animated arrow icon, reducing the vertical clutter in the extension popup.
Changes:
src/popup.html: Added a collapsible "Advanced Filters" container wrapping all filter checkboxes, with a toggle button and chevron arrow.src/scripts/popup.js: Added a click event listener for the toggle button to show/hide filter content and rotate the arrow icon.src/index.css: Added a.hiddenCSS rule (display: none).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
src/popup.html |
Wraps all filter checkboxes in a collapsible advanced-filters-container div with a toggle button and chevron icon |
src/scripts/popup.js |
Adds toggle click handler to show/hide filter content and rotate the chevron arrow |
src/index.css |
Adds a .hidden { display: none } rule (already exists in tailwindcss.css) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <button type="button" | ||
| id="toggleAdvancedFilters" | ||
| class="w-full border-2 border-gray-200 bg-gray-200 rounded-xl text-gray-800 p-2 pr-8 flex items-center justify-between focus:outline-none focus:ring-2 focus:ring-blue-500"> | ||
|
|
||
| <span class="flex items-center"> | ||
| Advanced Filters | ||
| </span> | ||
|
|
||
| <i id="advancedFiltersArrow" | ||
| class="fa fa-chevron-down text-gray-500 transition-transform duration-200"> | ||
| </i> | ||
| </button> |
There was a problem hiding this comment.
The new toggle button lacks aria-expanded and aria-controls attributes. These are standard accessibility attributes for collapsible/disclosure widgets: aria-expanded should reflect the current open/closed state (toggled by the JS click handler alongside the class changes), and aria-controls should reference the ID of the controlled content (advancedFiltersContent). Without these, assistive technology users cannot determine the state of or relationship between the button and the collapsible content.
src/popup.html
Outdated
| <i id="advancedFiltersArrow" | ||
| class="fa fa-chevron-down text-gray-500 transition-transform duration-200"> |
There was a problem hiding this comment.
The transition-transform and duration-200 utility classes used on the arrow icon (<i>) element are not defined in the local tailwindcss.css (which only bundles the subset of Tailwind utilities actually used in this project). Without these, the smooth CSS transition of the arrow rotation won't work. A transition: transform 0.2s ease-in-out; rule needs to be added for the arrow icon in src/index.css.
src/popup.html
Outdated
| <p class="font-medium text-sm"style="margin-bottom: 0.3rem;"> | ||
| Advanced Filters | ||
| </p> | ||
|
|
||
| <div class="relative mb-4" > | ||
| <button type="button" | ||
| id="toggleAdvancedFilters" | ||
| class="w-full border-2 border-gray-200 bg-gray-200 rounded-xl text-gray-800 p-2 pr-8 flex items-center justify-between focus:outline-none focus:ring-2 focus:ring-blue-500"> | ||
|
|
||
| <span class="flex items-center"> | ||
| Advanced Filters | ||
| </span> |
There was a problem hiding this comment.
The "Advanced Filters" label text appears redundantly twice: once in a <p> element immediately above the button (line 204–206), and once inside the button's <span> (line 213–215). This creates a visually duplicated heading and button label. One of these should be removed — typically the standalone <p> label above the button is unnecessary since the button itself already carries the label text.
src/popup.html
Outdated
| <p class="font-medium text-sm"style="margin-bottom: 0.3rem;"> | ||
| Advanced Filters | ||
| </p> | ||
|
|
||
| <div class="relative mb-4" > | ||
| <button type="button" | ||
| id="toggleAdvancedFilters" | ||
| class="w-full border-2 border-gray-200 bg-gray-200 rounded-xl text-gray-800 p-2 pr-8 flex items-center justify-between focus:outline-none focus:ring-2 focus:ring-blue-500"> | ||
|
|
||
| <span class="flex items-center"> | ||
| Advanced Filters | ||
| </span> |
There was a problem hiding this comment.
The "Advanced Filters" text strings in both the <p> label and the button <span> are hardcoded in English, while the rest of the popup's user-facing strings use the data-i18n attribute for internationalization (the project supports 18 locales). These strings should also be internationalized with a data-i18n attribute referencing a key defined in the locale files.
vedansh-5
left a comment
vedansh-5
left a comment
There was a problem hiding this comment.
Thanks @gurusatsangi for your contribution. Please pull in the latest changes in master branch and also address the comments mentioned below and copilots' review comments.
src/popup.html
Outdated
| <!-- Advanced Filters Wrapper --> | ||
| <div class="advanced-filters-container"> | ||
|
|
||
| <p class="font-medium text-sm"style="margin-bottom: 0.3rem;"> | ||
| Advanced Filters | ||
| </p> | ||
|
|
||
| <div class="relative mb-4" > |
There was a problem hiding this comment.
Please follow the same spacing between sections.
There was a problem hiding this comment.
@vedansh-5 Is the spacing correct tell me and i have removed advaced filters text appears redundantly twice. tell me show i can commit changes
src/popup.html
Outdated
| <button type="button" | ||
| id="toggleAdvancedFilters" | ||
| class="w-full border-2 border-gray-200 bg-gray-200 rounded-xl text-gray-800 p-2 pr-8 flex items-center justify-between focus:outline-none focus:ring-2 focus:ring-blue-500"> | ||
|
|
||
| <span class="flex items-center"> | ||
| Advanced Filters |
There was a problem hiding this comment.
please make this compatible for dark mode
There was a problem hiding this comment.
okay i will fixed full give me some time
There was a problem hiding this comment.
@vedansh-5 check this also is it correct or not.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
sorry for last two commit i have done it by mistake show please ignore it. |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider enhancing accessibility of the Advanced Filters toggle by wiring up
aria-expandedandaria-controlsto reflect the open/closed state and marking the chevron icon as decorative (e.g.,aria-hidden="true"). - The static
"Advanced Filters"label appears both as a<p>above and as the button text; you might simplify this by using a single semantic heading/label associated with the toggle to reduce redundancy and clarify structure for screen readers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider enhancing accessibility of the Advanced Filters toggle by wiring up `aria-expanded` and `aria-controls` to reflect the open/closed state and marking the chevron icon as decorative (e.g., `aria-hidden="true"`).
- The static `"Advanced Filters"` label appears both as a `<p>` above and as the button text; you might simplify this by using a single semantic heading/label associated with the toggle to reduce redundancy and clarify structure for screen readers.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
gurusatsangi
force-pushed
the
fix-collapsible-advanced-filters
branch
3 times, most recently
from
c0a0019 to
b6c108c
Compare
…fix-collapsible-advanced-filters
|
@vedansh-5 can you help me i have pushed but sourcery review find error many time please help me |
|
Hey, @gurusatsangi, seems the sourcery review failed since innerHTML is being used with user-controlled data. When you insert data using innerHTML, the browser interprets it as HTML, which can allow malicious scripts (XSS) to execute if the data contains <script> or event handlers. |
📌 Fixes
Fixes #385
📝 Summary of Changes
📸 Screenshots / Demo
Before
All filter checkboxes were visible by default, increasing popup height and making the layout visually cluttered.
After
Filter options are collapsed under an “Advanced Filters” section by default and can be expanded when needed, resulting in a cleaner and more compact popup layout.
✅ Checklist
👀 Reviewer Notes
This PR introduces a collapsible wrapper for the filter section to improve layout clarity and reduce vertical clutter in the popup. All existing functionality remains intact.
Summary by Sourcery
Make the popup’s filter section collapsible under an “Advanced Filters” toggle to reduce vertical clutter while preserving existing behavior.
New Features:
Enhancements:
Summary by Sourcery
Make the popup’s filter section collapsible under an Advanced Filters toggle to reduce vertical clutter while preserving existing behavior.
New Features:
Enhancements: